Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for (optionally) declaring the Pickle Protocol when writing to session store #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sayeghr
Copy link

@sayeghr sayeghr commented Jun 2, 2016

Added support for explicitly declaring the Pickle Protocol. This is to support sharing a session between flask apps using different python versions (2 or 3).

This pull request is to fix a specific problem when you have two flask apps, one written in Python 2 and one written in Python 3 that shares a server-side session. Python 3 uses Pickle Protocol 3 by default, but Python 2 can only read from Pickle Protocol 2 or less.

…o support sharing a session between flask apps using different python versions (2 or 3).
@mbr
Copy link
Owner

mbr commented Jun 3, 2016

Currently, the serialization code is supposed to be protocol-agnostic (you can swap it out with JSON or anything else), adding this configuration option would change that slightly and it is not a very common usecase.

Before merging this, have you looked at the alternative? Something like (untested code below, I hope it gets the idea across):

class SessionPickler(object):
    def __init__(self, protocol_version=2):
        self.protocol_version = protocol_version

    def loads(self, string):
        return pickle.loads(string)

    def dumps(self, obj):
        return pickle.dumps(obj, self.protocol_version)

KVSessionInterface.serialization_method = SessionPickler(2)

If you felt like it, you could get clever with inheritance and functools.partials =)

@sayeghr
Copy link
Author

sayeghr commented Jun 3, 2016

The KVSessionInterface class has the serialization_method hard coded with pickle, so I think it would be OK to have an optional way to pass in the pickling protocol you wish to use. If you want to use JSON instead you could simply omit the SESSION_PICKLE_PROTOCOL key from your flask config with no ill effects. Your proposed alternative solution would require monkey patching the KVSessionInterface at runtime which feels evil to me.

@mbr
Copy link
Owner

mbr commented Jun 3, 2016

The KVSessionInterface class has the serialization_method hard coded with pickle, so I think it would be OK to have an optional way to pass in the pickling protocol you wish to use.

I would disagree here, it's not hardcoded but a class variable. To me, hardcoded would be if I was calling pickle.dumps instead of self.serialization_method.dumps, for example.

The pattern is used across flask as well, have a look at https://github.com/pallets/flask/blob/c5900a1adf8e868eca745225f3cf32218cdbbb23/flask/sessions.py#L290:

class SecureCookieSessionInterface(SessionInterface):
    """The default session interface that stores sessions in signed cookies
    through the :mod:`itsdangerous` module.
    """
    #: the salt that should be applied on top of the secret key for the
    #: signing of cookie based sessions.
    salt = 'cookie-session'
    #: the hash function to use for the signature.  The default is sha1
    digest_method = staticmethod(hashlib.sha1)
    #: the name of the itsdangerous supported key derivation.  The default
    #: is hmac.
    key_derivation = 'hmac'
    #: A python serializer for the payload.  The default is a compact
    #: JSON derived serializer with support for some extra Python types
    #: such as datetime objects or tuples.
    serializer = session_json_serializer
    session_class = SecureCookieSession

salt, digest_method, key_derivation, serializer and session_class are all overridable class variables, for this reason they are also well documented. The latter not being the based for Flask-KVSession is probably a shortcoming here.

@sayeghr
Copy link
Author

sayeghr commented Jun 3, 2016

Okay, I agree with you but it would be nice to be able to set the pickling protocol within your library without having to create your own pickler subclass. Especially since your default implementation uses pickle and already accepts optional variables from the flask app configuration.

I will implement the pickler subclass in my codebase if you do not want to accept this pull request.

@sayeghr
Copy link
Author

sayeghr commented Jun 7, 2016

Is there anything I can add to this PR to make you more willing to accept it? One option would be to check if the serialization_method is of the pickle class before respecting the SESSION_PICKLE_PROTOCOL config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants